Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IMX219: Adjust PLL settings based on the number of MIPI lanes #6575

Open
wants to merge 1 commit into
base: rpi-6.6.y
Choose a base branch
from

Conversation

peyton-howe
Copy link

Still can tweak the exact PLL settings, but this adds the overlays and driver support needed for 4-lane operation. I have tested on my Pi5 and run with both 4-lane and 2-lane cameras simultaneously.

@6by9
Copy link
Contributor

6by9 commented Jan 1, 2025

Links to https://forums.raspberrypi.com/viewtopic.php?t=381663

For reference, driver changes and overlay changes need to be in separate patches.

The overlay can be an override on the existing imx219 overlay to reduce duplication. My diff (which I can't test) doing that was

diff --git a/arch/arm/boot/dts/overlays/imx219-overlay.dts b/arch/arm/boot/dts/overlays/imx219-overlay.dts
index 4c4bcd309a3d..3411d5bbd8ce 100644
--- a/arch/arm/boot/dts/overlays/imx219-overlay.dts
+++ b/arch/arm/boot/dts/overlays/imx219-overlay.dts
@@ -65,6 +65,21 @@ csi_ep: endpoint {
                };
        };
 
+       fragment@201 {
+               target = <&csi_ep>;
+               __dormant__ {
+                       data-lanes = <1 2 3 4>;
+               };
+       };
+
+       fragment@202 {
+               target = <&cam_endpoint>;
+               __dormant__ {
+                       data-lanes = <1 2 3 4>;
+                       link-frequencies =
+                               /bits/ 64 <363000000>;
+       };
+
        __overrides__ {
                rotation = <&cam_node>,"rotation:0";
                orientation = <&cam_node>,"orientation:0";
@@ -77,6 +92,7 @@ __overrides__ {
                       <&vcm>, "VANA-supply:0=", <&cam0_reg>;
                vcm = <&vcm>, "status=okay",
                      <&cam_node>,"lens-focus:0=", <&vcm>;
+               4lane = <0>, "+201+202";
        };
 };

imx219->lanes == 2 ? imx219_pll : imx219_pll_4lane,
imx219->lanes == 2 ? ARRAY_SIZE(imx219_pll) : ARRAY_SIZE(imx219_pll_4lane),
NULL);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not handling any failures from this table write.

TBH you may as well move the IMX219_REG_CSI_LANE_MODE register into the array and just return the value from cci_multi_reg_write, or pass &ret as the last argument to cci_write below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied your recommended changes in the latest commit

@pelwell
Copy link
Contributor

pelwell commented Jan 1, 2025

There's a missing line (another closing brace and semicolon) in the proposed fragment 202, but otherwise it looks okay.

@peyton-howe peyton-howe changed the title IMX219: Add overlay and fix PLL settings for 4-lane operation IMX219: Update PLL settings based on the number of MIPI lanes Jan 4, 2025
@peyton-howe
Copy link
Author

Removed overlay changes from this PR and updated driver based on 6by9's suggestion

@pelwell
Copy link
Contributor

pelwell commented Jan 4, 2025

Can you give us a Signed-off-by line for the changes in your PRs (this and #6580)? Mine would be: Signed-off-by: Phil Elwell <[email protected]>

@peyton-howe peyton-howe changed the title IMX219: Update PLL settings based on the number of MIPI lanes IMX219: Adjust PLL settings based on the number of MIPI lanes Jan 5, 2025
@peyton-howe
Copy link
Author

Done for both!

@@ -1553,4 +1574,4 @@ module_i2c_driver(imx219_i2c_driver);

MODULE_AUTHOR("Dave Stevenson <[email protected]");
MODULE_DESCRIPTION("Sony IMX219 sensor driver");
MODULE_LICENSE("GPL v2");
MODULE_LICENSE("GPL v2");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is spurious and makes checkpatch sad.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now :)

@pelwell
Copy link
Contributor

pelwell commented Jan 10, 2025

Any comments, @6by9?

@6by9
Copy link
Contributor

6by9 commented Jan 13, 2025

I'd hoped to test with a module, but it got stuck in customs (hopefully arriving tomorrow).

Commit message could be fuller, and include Fixes: ceddfd4493b3 ("media: i2c: imx219: Support four-lane operation"), but I can do that when upstreaming it.

Do the pixel rate numbers work out for frame rate control?
That upstream commit changes the pixel rate to 280.8MPix/s (from 184.2) and link frequency of 363MHz (from 456MHz). Both those numbers come from section "9-2 clock setting example" in the datasheet (see https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS/blob/master/RASPBERRY%20PI%20CAMERA%20V2%20DATASHEET%20IMX219PQH5_7.0.0_Datasheet_XXX.PDF).

sohonomura2020's post at https://forums.raspberrypi.com/viewtopic.php?p=2283325#p2283325 implies the new IMX219_REG_PLL_VT_MPY should be 0x57 = 87 and IMX219_REG_PLL_OP_MPY should be 0x5a = 90. Your patch has 88 and 91.

My maths says that IMX219_REG_PLL_OP_MPY = 91 gives a link rate of 364MHz, whilst 90 gives a link rate of 360MHz. Neither is the driver value of 363MHz.
Likewise for IMX219_REG_PLL_VT_MPY, =88 gives a pixel rate of 281.6MPix/s, whilst 87 gives a pixel rate of 278.4MPix/s. Neither is the driver value of 280.8MPix/s.
So whilst your numbers will improve matters, they still look slightly off. Frame rates and exposure times will likewise be slightly wrong.

The example does say EXCK_FREQ = 12MHz.
702/12 = 58.5, so I think the only way that the example timings can be achieved is by INCK=12MHz, Pre-Div1 = 2, and then PLL_VT_MPY = 117.
We have INCK = 24MHz. There is no Pre-Div1 option of /4 listed (although it is defined as an 8 bit register, so might be worth a quick test), so I don't think the numbers can be achieved.

Continuing the search, registers 0x110A/B are listed as max_pre_pll_clk_div / "Maximum Pre PLL divider value" with a value of 0x0D (14 decimal), so perhaps /4 is permitted. That would be worth a test, as amending the link frequency is a pain (we'd have to log a warning for the approximation if asked for the previously "supported" value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants